Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup on console closed & log error.stack #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

olive75
Copy link

@olive75 olive75 commented Aug 7, 2014

Added log of error.stack and cleanup when the Windows console close. 'Exit' seems not to be the right event to take async actions (from http://nodejs.org/api/process.html#process_event_exit), so I removed it.

Added log of error.stack and cleanup when the Windows console close.
@FREEZX
Copy link
Owner

FREEZX commented Aug 8, 2014

This really feels like the wrong way to do cleanup. None of the exit events work asynchroniously, so it's always messy, and data is left.

@dbrugne
Copy link

dbrugne commented Sep 25, 2014

@FREEZX , maybe I miss-understand how Node works and exit.

But I have understood that process.on('SIGINT') handler can perform asynchronous calls.

So I tested:

// client => Redis client
process.on('SIGINT', function() {
  console.log('Process shutdown by SIGINT');
  var now = new Date().toString();
  console.log('Try to write test-shutdown key with '+now);
  client.set('test-shutdown', now, function(err, set) {
    console.log('Is written: '+set+', now exit');
    process.exit();
  });
});

And I get (Windows console):

Process shutdown by SIGINT
Try to write test-shutdown key with Thu Sep 25 2014 13:56:16 GMT+0200 (Paris, Madrid (heure d’été))
Is written: OK, now exit

The key is correctly created in Redis store.
Where I'm wrong?

I've also another question. What do you mean by "normal exit": I didn't manage to do the exit cleanup correctly, so after normal exit, rooms are still... in socketio#15 ?

@FREEZX
Copy link
Owner

FREEZX commented Sep 25, 2014

SIGINT is only called when you do ctrl+c.
By normal exit i mean application exit without a specific intervention by the user.
For example, a batch job that runs for an amount of time and then exits by itself.
In that case you only get the exit event callback, which does not support async calls.
Correct me if i'm wrong, but i think that currently there's no good way of handling this type of 'normal' exit.
Sorry if my previous comment was incorrect, what i really meant was that not all exit events work well.

@dbrugne
Copy link

dbrugne commented Sep 26, 2014

@FREEZX OK I get it.

I agree with you, no way to handle this type of exit (direct process.exit() call) or (obviously) power unpluged or machine destruction.
So I think we can change the README disclaimer to precise that the actual limitation on exit cleanup concern only side use case and crash. In a normal usage (and on Linux like) platforms the problem is less critical (but exists).

Later we can maybe add an automatic cleanup logic on restart/other nodes detection but I think it add a complex and heavy logic over the module.

@FREEZX
Copy link
Owner

FREEZX commented Sep 30, 2014

@dbrugne That's the only logical solution i can think of. I wonder how 0.9.x handled this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants